Claude finished @phernandez's task in 2m 32s —— View job

Code Review

• Gather context and understand PR
• Review plugin manifest and marketplace entry
• Review hook scripts (session-start.sh, pre-compact.sh)
• Review validator script
• Review skills structure and SKILL.md files
• Check for hardcoded paths / sensitive data
• Verify justfile changes
• Post comprehensive review

Summary

This PR adds a well-structured Codex-native plugin package for Basic Memory. The architecture is solid — proper hook scripts, validator, skills with frontmatter, schemas, and clean justfile integration. There is one critical blocker that mirrors what the Codex automated review flagged, plus a few minor items.

Team Checklist

Critical Issue

❌ P1: Missing plugins/codex/.mcp.json Fix this →

plugin.json:22 declares "mcpServers": "./.mcp.json" and the validator at scripts/validate_codex_plugin.py:74 does:

mcp = read_json(plugin_dir / ".mcp.json")
servers = mcp.get("mcpServers")
if not isinstance(servers, dict) or "basic-memory" not in servers:
    raise SystemExit(".mcp.json: expected mcpServers.basic-memory")
...
if basic_memory.get("command") not in {"uvx", "basic-memory", "bm"}:
    raise SystemExit(".mcp.json: basic-memory server uses an unexpected command")

plugins/codex/.mcp.json does not exist — just package-check-codex fails immediately with Missing JSON file: .../plugins/codex/.mcp.json, and Codex itself cannot load the Basic Memory MCP server when the plugin is installed.

The file needs to be created. Based on bm-setup/SKILL.md:19 ("The plugin bundles an .mcp.json that starts uvx basic-memory mcp"), the expected content is something like:

{
  "mcpServers": {
    "basic-memory": {
      "command": "uvx",
      "args": ["basic-memory@latest", "mcp"]
    }
  }
}

Minor Items

⚠️ skills-lock.json deleted without explanation

The root-level skills-lock.json was deleted. It tracked the instrumentation skill sourced from pydantic/skills. No code references it now, but the PR description doesn't mention why it was removed. If this was intentional (e.g., skill management moved elsewhere), a note in the PR would be helpful.

ℹ️ pre-compact.sh: naive datetime.now() (no timezone)

pre-compact.sh:147:

now = datetime.now()
iso = now.strftime("%Y-%m-%dT%H:%M:%S")

This produces timestamps in the local system timezone with no UTC offset, which can be ambiguous on servers with non-UTC locales. Since the codex-session schema uses a plain string type for started/ended, this is a minor point, but datetime.now(timezone.utc) (with from datetime import timezone) would be more portable.

ℹ️ session-start.sh: deduplication between different-type queries (harmless)

session-start.sh:165:

session_rows = [r for r in rows(primary_sessions) if r not in codex_rows]

codex_rows searches type=codex_session and session_rows searches type=session. These types are mutually exclusive in practice, so the dedup check is always a no-op. Harmless, but slightly misleading.

ℹ️ hooks.json: SessionStart timeout vs. inner search timeouts

hooks.json:13 sets a 30-second outer timeout for SessionStart. The embedded Python runs up to 4 + MAX_SHARED parallel searches, each with a 10-second inner timeout. For the default config this should be fine, but with 6 shared projects all hitting slow cloud reads simultaneously, the inner retries could bump against the outer wall. Low risk but worth noting.

What Works Well

• Best-effort hook contract is properly enforced — both scripts exit 0 on every failure path, so a missing Basic Memory install or slow network never blocks Codex.
• Parallel search in session-start.sh using ThreadPoolExecutor is good for performance.
• Validator is thorough — checks manifest fields, MCP config structure, hook executability, skill frontmatter, and schema type/entity fields.
• CODEX_PLUGIN_VALIDATOR opt-in for the scaffold validator is a clean pattern — required for CI gate, optional for contributors without the full SDK.
• Hook scripts probe basic-memory → bm → uvx → uv tool run — good graceful degradation across install methods.
• No hardcoded paths — verified clean.

The only merge blocker is the missing plugins/codex/.mcp.json. Once that file is added, just package-check-codex should pass cleanly.

Claude finished @phernandez's task in 2m 4s —— View job

Code Review

• Gather context and understand PR
• Review plugin manifest and MCP config
• Review hook scripts (session-start.sh, pre-compact.sh)
• Review validator script and tests
• Review skills structure and SKILL.md files
• Check hardcoded paths / sensitive data
• Verify justfile changes and marketplace entry
• Post comprehensive review

Summary

This PR adds a well-structured Codex-native Basic Memory plugin. The previous critical issue (missing .mcp.json) has been resolved in the follow-up commit 092d2d7, and both minor concerns from the automated review (naive datetime.now(), deduplication logic) were also corrected. The new tests in tests/test_codex_plugin_package.py explicitly assert all three fixes. The package is clean and ready for merge with a few minor notes below.

Team Checklist

Findings

ℹ️ P3: .mcp.json uses basic-memory without a version pin

plugins/codex/.mcp.json:5:

"args": ["basic-memory", "mcp"]

Running uvx basic-memory mcp (no @latest) uses whatever uvx has cached, which may be stale for a first-time install that hits a pre-existing cache entry. The bm-setup/SKILL.md:19 explicitly says the plugin starts uvx basic-memory mcp (no version), so this is consistent — but for a distributed plugin, basic-memory@latest would ensure a fresh resolve. Low risk since Codex users will typically have a recent install anyway.

ℹ️ P3: validate_codex_plugin.py hardcodes name == "codex" check

scripts/validate_codex_plugin.py:57:

if manifest.get("name") != "codex":
    raise SystemExit(f"{manifest_path}: expected name=codex")

This makes the validator Codex-specific rather than general-purpose. Fine for now since it's only wired to plugins/codex, but if the pattern expands to other Codex-family plugins, this will need to be parameterized. No action needed today.

ℹ️ P3: pre-compact.sh writes cwd into note frontmatter

plugins/codex/hooks/pre-compact.sh:157:

f"cwd: {cwd}",

cwd is an absolute path from the Codex environment. This is fine for local use but in a shared/cloud Basic Memory project this leaks the user's filesystem layout into shared notes. Since bm-share (bm-share/SKILL.md) explicitly guards cross-project writes, this is low risk, but worth a comment in the code. No blocking concern.

✅ Previous Issues — All Resolved

The three issues flagged in the prior automated and Claude review are confirmed fixed:

What Works Well

• Best-effort contract is solid across both hooks — every failure path exits 0.
• shlex.split() for BM_BIN correctly handles multi-word commands like "uv tool run basic-memory".
• ThreadPoolExecutor in session-start.sh with max_workers=4 + MAX_SHARED is efficient and capped.
• Validator is thorough — checks manifest, MCP config, hook executability, skill frontmatter, and schema type/entity.
• Focused test assertions in test_codex_plugin_package.py pin behavioral contracts rather than just structure.
• bm-setup/SKILL.md is notably detailed — structured interview flow, explicit confirmation before seeding schemas, verification step before finishing.

No merge blockers. The three previously identified issues are resolved. The notes above are all informational (P3) and don't require changes before merging.

Claude finished @phernandez's task in 2m 27s —— View job

Code Review

• Gather context and understand PR
• Review plugin manifest and .mcp.json
• Review hook scripts
• Review validator and tests
• Review skills structure
• Check hardcoded paths / sensitive data
• Verify justfile changes and checklist
• Post comprehensive review

Summary

This PR adds a well-structured Codex-native Basic Memory plugin. The previously-flagged critical blocker (missing .mcp.json) was resolved in 092d2d7, and the three issues from the prior reviews (missing MCP config, naive datetime.now(), cross-type dedup no-op) are all confirmed fixed. The test in tests/test_codex_plugin_package.py pins all three behavioral contracts.

There is one open question needing author clarification before merging, plus a few minor notes.

Team Checklist

Open Question: .mcp.json key format

⚠️ Needs author input before merging

The Codex automated reviewer (P1) flags that .mcp.json uses the Claude-style mcpServers (camelCase) wrapper, and says Codex expects either a direct server map or mcp_servers (snake_case).

Current plugins/codex/.mcp.json:

{
  "mcpServers": {
    "basic-memory": { ... }
  }
}

The internal validator (scripts/validate_codex_plugin.py:75) was written to match:

servers = mcp.get("mcpServers")

So both the file and the validator are internally consistent. The question is whether the Codex runtime itself recognizes mcpServers or requires mcp_servers / a flat map. If Codex uses mcpServers (same as Claude Code and many MCP clients), the current format is correct and the Codex bot is wrong. If Codex uses a different key, both the file and the validator need updating together.

@phernandez — can you confirm which key the Codex platform actually reads from .mcp.json? If you've tested this locally with Codex and the MCP server connects successfully, that closes the question. If not, this is worth verifying before merging so the distributed plugin actually connects the MCP server.

Minor Notes (P3, no blocking action needed)

plugins/codex/.mcp.json:5 — no version pin

"args": ["basic-memory", "mcp"]

uvx basic-memory (no @latest) will use a cached version on repeat installs. bm-setup/SKILL.md:19 explicitly uses this form, so it's consistent by design. For a distributed plugin, basic-memory@latest would ensure a fresh resolve on first install. Low risk — Codex users typically have a recent install.

plugins/codex/hooks/pre-compact.sh:157 — cwd in YAML frontmatter

f"cwd: {cwd}",

cwd is an absolute path from the Codex environment. For cloud Basic Memory projects (shared with teammates), this writes the local filesystem layout into a shared note. Since bm-share guards cross-project writes, the risk is low, but a comment here would help future maintainers understand the tradeoff.

scripts/validate_codex_plugin.py:57 — hardcoded name == "codex"

if manifest.get("name") != "codex":
    raise SystemExit(f"{manifest_path}: expected name=codex")

Makes the validator Codex-specific rather than general-purpose. Fine today since it's only wired to plugins/codex, but worth noting for when/if the pattern expands.

What Works Well

• bm-setup/SKILL.md is notably thorough — structured interview, explicit confirmation before seeding schemas, verification step before finishing.
• shlex.split() for BM_BIN correctly handles multi-word commands like "uv tool run basic-memory".
• ThreadPoolExecutor(max_workers=4 + MAX_SHARED) in session-start.sh is capped and efficient. With MAX_SHARED=6 and a 30s outer timeout vs 10s inner per search running in parallel, timing is safe.
• Graceful degradation — both hooks probe basic-memory → bm → uvx → uv tool run before giving up.
• Schema design — codex-session.md using validation: warn means checkpointing never blocks the user flow.
• .gitignore negation pattern (!/plugins/codex/.mcp.json) is the correct way to un-ignore a specific file within a wildcard rule.
• Focused test assertions pin behavioral contracts rather than just structure.

One point needs author confirmation (.mcp.json key format). Everything else is clean. Once that's resolved, no merge blockers remain.

Claude finished @phernandez's task in 2m 29s —— View job

Code Review

• Gather context and understand PR
• Review plugin manifest and .mcp.json format
• Review hook scripts (session-start.py, pre-compact.py)
• Review validator script and tests
• Review skills structure and SKILL.md files
• Check hardcoded paths / sensitive data
• Verify justfile changes and marketplace entry
• Address open .mcp.json key format question
• Post comprehensive review

Summary

This PR adds a well-structured Codex-native Basic Memory plugin. Three of the four issues raised in prior automated reviews are confirmed resolved. There are two items needing attention before merging: the open .mcp.json key-format question (previously flagged P1 by the Codex bot, still unresolved) and a missing clear source in the SessionStart hook matcher (P2 from the latest Codex bot pass).

Team Checklist

Items Needing Attention

⚠️ P1: Confirm .mcp.json key format (mcpServers vs mcp_servers)

The Codex automated review (commits 092d2d7, aa51d7) flags P1 that Codex expects either a flat server map or mcp_servers (snake_case), not mcpServers (camelCase from the Claude Code MCP convention).

Current plugins/codex/.mcp.json:

{
  "mcpServers": {
    "basic-memory": { "command": "uvx", "args": ["basic-memory", "mcp"] }
  }
}

Internal validator scripts/validate_codex_plugin.py:80 reads mcp.get("mcpServers") — the file and validator are internally consistent. The question is whether the Codex platform runtime reads mcpServers, mcp_servers, or a flat map.

If Codex doesn't recognize mcpServers, the MCP server never starts and all skills that rely on it (bm-orient, bm-checkpoint, etc.) operate in degraded fallback mode with no MCP tools available.

If mcpServers is correct (Codex inherited the same convention as Claude Code), no changes needed — close this.

If Codex requires mcp_servers, update both:

1. plugins/codex/.mcp.json:2: "mcpServers" → "mcp_servers"
2. scripts/validate_codex_plugin.py:80-81: mcp.get("mcpServers") → mcp.get("mcp_servers")

@phernandez — have you loaded this plugin in Codex locally and confirmed the Basic Memory MCP server connects? That would close this definitively.

⚠️ P2: Add clear to SessionStart hook matcher

plugins/codex/hooks/hooks.json:6 — the Codex hooks docs list startup|resume|clear|compact as the four valid startup sources. The current matcher is startup|resume|compact, skipping clear. When a user starts a fresh context with /clear, the orientation hook doesn't fire and they don't get the Basic Memory briefing.

Current:

"matcher": "startup|resume|compact"

Should be: Fix this →

"matcher": "startup|resume|clear|compact"

Minor Notes (P3, informational)

.mcp.json — no version pin

plugins/codex/.mcp.json:6: "args": ["basic-memory", "mcp"] — uvx basic-memory without @latest uses whatever uvx has cached. This is consistent with bm-setup/SKILL.md:19 by design. basic-memory@latest would force a fresh resolve on first install. Low risk for the typical Codex user.

pre-compact.py:160 — cwd in YAML frontmatter

f"cwd: {cwd}",

cwd is an absolute path from the Codex environment. For users with cloud Basic Memory projects shared with teammates, this writes the local filesystem layout into a shared note. The bm-share skill guards cross-project writes, so the risk is low, but a comment near this line would help future maintainers understand the tradeoff.

validate_codex_plugin.py:62 — hardcoded name == "codex"

if manifest.get("name") != "codex":

Validator is Codex-specific — fine today since it's only wired to plugins/codex. Worth a note for if the pattern expands to other Codex-family plugins.

What Works Well

• Best-effort contract enforced correctly — both hooks exit 0 on every failure path, including missing Basic Memory install, empty project, and slow cloud reads.
• shlex.split() for BM_BIN correctly handles multi-word commands like "uv tool run basic-memory".
• ThreadPoolExecutor(max_workers=4 + MAX_SHARED) in session-start.py with 10-second per-search timeouts fits inside the 30-second outer hooks.json timeout when searches run in parallel.
• elif session_rows pattern in session-start.py:195 cleanly replaces the invalid cross-type dedup that was there before, and the test asserts its absence.
• bm-setup/SKILL.md is notably thorough — structured interview flow, explicit confirmation before seeding schemas, verification step before finishing.
• --after_date flag (with underscore) is correct — confirmed in src/basic_memory/cli/commands/tool.py:391.
• .gitignore negation !/plugins/codex/.mcp.json is the correct way to un-ignore a specific file within a wildcard rule.
• Focused test assertions in test_codex_plugin_package.py pin behavioral contracts (UTC pattern, dedup removal, doc content) rather than just file existence.

Two items need resolution before merging: confirm the .mcp.json key format is recognized by the Codex runtime (P1), and add clear to the SessionStart matcher (P2). Everything else is clean.